Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove custom arguments in object.__new__ of BaseInstrumentor #1439

Merged
merged 33 commits into from
Feb 28, 2023
Merged

Remove custom arguments in object.__new__ of BaseInstrumentor #1439

merged 33 commits into from
Feb 28, 2023
+52 −1

Conversation

rbagd
Copy link
Contributor

@rbagd rbagd commented Nov 11, 2022 •
edited
Loading

Description

Classes inheriting from BaseInstrumentor class are singletons tracked by _instance attribute and this piece of code

    def __new__(cls, *args, **kwargs):
        if cls._instance is None:
            cls._instance = object.__new__(cls, *args, **kwargs)

If an instrumentor child class with parameters, e.g. SystemMetricsInstrumentor is initialized first, then these parameters are also passed to object.__new__ which results in an error as in #1438.

# This works
SystemMetricsInstrumentor()
SystemMetricsInstrumentor(config={})

# This does not
SystemMetricsInstrumentor(config={})
SystemMetricsInstrumentor()

Given the answers in this SO question, I am inclined to think that it's not needed to pass arguments to object.__new__ and they can be safely removed. Those arguments will still end up in the __init__ call.

Fixes #1438

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added a unit test in system metrics to reproduce the issue (this requires singleton class to be reset during test run)
  • Additional tests were ran with toxfor multiple other instrumented packages

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rbagd rbagd requested a review from a team November 11, 2022 22:30
@ocelotl
Copy link
Contributor

ocelotl commented Nov 18, 2022

Nice, please add another test case, one that pretty much does this and checks no exception is raised:

def test_some_test_case_name(self):
    try:
        SystemMetricsInstrumentor()
        SystemMetricsInstrumentor(config={})
    except Exception as error:
        self.fail(f"Unexpected exception {error} raised")
    # do any necessary resetting
        try:
        SystemMetricsInstrumentor()
        SystemMetricsInstrumentor(config={})
    except Exception as error:
        self.fail(f"Unexpected exception {error} raised")

@rbagd
Copy link
Contributor Author

rbagd commented Nov 18, 2022

I added an extra case as per @ocelotl request. I assumed he meant the init order should be inverted in the second case. 😉

@ocelotl
Copy link
Contributor

ocelotl commented Nov 21, 2022

I added an extra case as per @ocelotl request. I assumed he meant the init order should be inverted in the second case. wink

right! 😅 some tests are failing, please take a look

@rbagd
Copy link
Contributor Author

rbagd commented Nov 21, 2022

All failing tests I've checked were related to #1460. Now that it got merged, I think it should be alright.

@rbagd
Copy link
Contributor Author

rbagd commented Dec 5, 2022

Both failing jobs failed due to some issue with git itself. Is it possible to re-run just those two?

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +95327fbe060f4136c197c59869fb13484d3b77c8:refs/remotes/pull/1439/merge
Error: fatal: protocol error: bad pack header
Error: The process '/usr/bin/git' failed with exit code 128

@srikanthccv
Copy link
Member

Both failing jobs failed due to some issue with git itself. Is it possible to re-run just those two?

This should be fine now.

@srikanthccv
Copy link
Member

@ocelotl PTAL

@rbagd
Copy link
Contributor Author

rbagd commented Feb 22, 2023

@ocelotl @srikanthccv Linting failed due to a style change in black. I think I got it fixed so if possible to get this merged before they update it again, it'd be great. :)

srikanthccv
srikanthccv reviewed Feb 23, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@srikanthccv
Copy link
Member

@ocelotl PTAL

@ocelotl ocelotl merged commit 4a859e3 into open-telemetry:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv left review comments

@ocelotl ocelotl ocelotl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

System metrics instrumentation not working with custom defined configuration
4 participants
@rbagd @ocelotl @srikanthccv @lzchen